Skip to content

fix(sonic): emit up/down for BGP_NEIGHBOR_AF - #2618

Open
ideaship wants to merge 1 commit into
mainfrom
sonic-bgp-af-admin-status
Open

fix(sonic): emit up/down for BGP_NEIGHBOR_AF#2618
ideaship wants to merge 1 commit into
mainfrom
sonic-bgp-af-admin-status

Conversation

@ideaship

@ideaship ideaship commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Problem

The SONiC config generator wrote {"admin_status": "true"} for every
BGP_NEIGHBOR_AF row. That field is typed stypes:admin_status in
the AF grouping, and the typedef is a strict enumeration:

typedef admin_status {
    description "Administrative status (up/down)";
    type enumeration { enum up; enum down; }
}

sonic-types.yang.j2#L88-L94

So "true" is outside the type, not a lenient spelling of it. Upstream's own
YANG test config for this table uses "up", with a real boolean
(send_default_route) immediately beside it, so the model distinguishes
deliberately: tests_config/bgp.json#L387-L395

Every other table this generator writes — PORT, PORTCHANNEL, LOOPBACK,
VLAN, VLAN_INTERFACE and the MGMT_ tables — already emits up/down.
BGP_NEIGHBOR_AF was the lone outlier.

Why it is not cosmetic

config reload runs YANG validation and aborts on failure, via
config_file_yang_validation. On sonic-utilities master the
no-filename form validates /etc/sonic/config_db.json:

if filename is None and file_format == 'config_db':
    if not force:
        ... if not config_file_yang_validation(cfg_file): raise click.Abort()

config/main.py#L2226-L2258

That is exactly how this project applies a generated config (config load
followed by config reload -y), so "true" becomes a hard failure in a
future release.

On 202505 the validator exists but is reached only under
if filename is not None, the explicit-filename path:
config/main.py#L1961-L1967. So an explicit-filename reload
already aborts today, while the no-filename form does not. The block quoted
above is what master adds.

Every link is pinned to a commit SHA so the line ranges stay valid:
sonic-buildimage 8aaea5e3649a, sonic-utilities
e3768cbdc0f4 and 7511bf2f8ea3 (202505).

Impact

Behaviour on the switch is unchanged: frrcfgd's hdl_admin_status converts
up/down to true/false internally, so the consumer already expects this
spelling: frrcfgd.py#L1456-L1466

Switches running a previously generated config hold "true" and will show the
new value as a diff the next time their config is regenerated. Harmless, for
the same reason.

The bundled ConfigDB validator rejected every generated AF row before this
change (Input should be 'up' or 'down') and accepts them now. That removes the
largest source of noise from osism sonic validate for these tables, but the
command is not yet usable as a gate: PORT.adv_speeds is modelled as a list
while ConfigDB carries a string, and BGP_NEIGHBOR.local_addr is flagged
against its union's leafref arms while a plain inet:ip-address arm makes the
value legal. Both are separate.

Tests

Twelve assertions in the BGP test module are updated to match. 712 passed in
tests/unit/tasks/conductor/sonic/, 3139 passed in tests/unit (4
pre-existing xfails), flake8 and black clean.

Note for reviewers

No committed golden pins the old value: the SONiC E2E goldens are not in
main, they exist only on the draft stack rooted at:

The matching golden update belongs to that stack and lands with it, so this
change breaks nothing currently in main.

🤖 Generated with Claude Code

The generator wrote {"admin_status": "true"} for every BGP_NEIGHBOR_AF
row. The ConfigDB YANG typedef admin_status is a strict enumeration of
up and down (sonic-types.yang.j2), so "true" is outside the type, and
upstream's own YANG test config for this table uses "up". Every other
table the generator writes -- PORT, PORTCHANNEL, LOOPBACK, VLAN,
VLAN_INTERFACE and the MGMT_ tables -- already emits up/down, which
left BGP_NEIGHBOR_AF as the lone outlier.

This is not cosmetic. config reload runs YANG validation and aborts on
failure. On sonic-utilities master the no-filename form validates
/etc/sonic/config_db.json, which is exactly how this project applies a
generated config, so "true" becomes a hard failure in a future
release; on 202405 and later an explicit-filename reload already
aborts.

Behaviour on the switch is unchanged, because frrcfgd converts up and
down to true and false internally. Switches already running a
generated config hold "true" and will show the new value as a diff the
next time their config is regenerated.

The bundled validator rejected every generated AF row before this
change and accepts them now, which is what allows osism sonic validate
to be used as a gate.

Twelve assertions in the BGP test module are updated to match.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Roger Luethi <luethi@osism.tech>

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!

Sourcery assessment

Needs a human reviewer. This changes the administrative state value emitted for every affected BGP neighbor address family, which could activate or fail to activate routing sessions and cause route withdrawal or a network outage. Reverting restores the previous generated configuration, but any traffic disruption or route changes that occurred before the revert are not undone.


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Ready for review

Development

Successfully merging this pull request may close these issues.

2 participants